Complete fix for IndexError and reference level logic in get_musical_time()#32
Merged
Complete fix for IndexError and reference level logic in get_musical_time()#32
Conversation
…time() This comprehensive fix addresses the critical IndexError bugs reported in Issue #31: 1. **IndexError Resolution**: - Added sparse pulse detection and proportional timing fallback to pulse-based calculation path - Fixed bounds checking when accessing all_pulses array - Both default mode and reference_level=2 now work correctly with sparse pulse data 2. **Reference Level Logic Correction**: - Fixed fundamental misunderstanding of reference level semantics - Reference levels now correctly calculate fractional position within the CONTAINING unit: * Reference Level 0: fractional position within the current CYCLE * Reference Level 1: fractional position within the current BEAT * Reference Level 2: fractional position within the current SUBDIVISION - Updated both proportional timing fallback and pulse-based calculation paths 3. **Specification Update**: - Updated docs/musical-time-spec.md to reflect corrected reference level behavior - Fixed test case examples to show proper fractional_beat calculations - Clarified semantic descriptions for reference level behavior 4. **Comprehensive Testing**: - Verified fix works for both sparse pulse data (real transcriptions) and complete pulse data (synthetic meters) - IndexError reproduction cases now return successful MusicalTime results - All reference levels produce correct fractional_beat values The fix ensures that get_musical_time() works reliably across all scenarios while maintaining the correct musical semantics for reference levels. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Updates the 5 failing tests to reflect the corrected reference level behavior: 1. **test_regular_meter_default_level**: Updated to expect fractional_beat=0.5 (halfway between subdivisions in default mode) 2. **test_reference_level_beat**: Updated to expect fractional_beat=0.375 (37.5% through cycle with reference_level=0) 3. **test_reference_level_subdivision**: Updated to expect fractional_beat=0.5 (50% through beat with reference_level=1) 4. **test_recursive_overflow_edge_case**: Updated to expect fractional_beat=0.4995 (49.95% through cycle with reference_level=0) 5. **test_fractional_beat_distribution_with_reference_level_zero**: Updated range expectation from >0.3 to >0.15 to match corrected cycle-based logic All tests now pass with the corrected reference level semantics: - Reference Level 0: fractional position within current CYCLE - Reference Level 1: fractional position within current BEAT - Reference Level 2: fractional position within current SUBDIVISION Full test suite: 343 tests passing ✅ 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
jon-myers
force-pushed
the
fix-fractional-beat-clustering
branch
from
c507918 to
0cf3453
Compare
Contributor
|
📦 Test Package Built Successfully! This PR has been automatically built and uploaded to TestPyPI for testing. 🔗 TestPyPI Link: https://test.pypi.org/project/idtap/ To test this version: pip install --index-url https://test.pypi.org/simple/ idtap✅ All tests passed and package builds successfully. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This comprehensive fix addresses the critical IndexError bugs reported in Issue #31 and corrects the fundamental reference level logic in
get_musical_time().🐛 Critical Bugs Fixed
all_pulsesarray🔧 Reference Level Logic Correction
Fixed fundamental misunderstanding of reference level semantics:
Previously incorrectly calculated fractional position within the unit AT the reference level instead of within the CONTAINING unit.
📋 Technical Changes
docs/musical-time-spec.mdto reflect corrected behavior🧪 Testing Results
📊 Impact
🔗 Related Issues
Closes #31
🤖 Generated with Claude Code